Platform compatibility analyzer draft #3

buyaa-n · 2020-07-07T22:58:45Z

Related design doc https://github.com/dotnet/designs/blob/master/accepted/2020/platform-checks/platform-checks.md

Related issue dotnet/runtime#37359 (It need to be updated with latest API review changes)

src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md

…nto plat_spec_an

overload of guard method

.../UnitTests/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzerTests.cs

src/Test.Utilities/CSharpCodeFixVerifier`2.cs

...yzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.Data.cs

.../UnitTests/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzerTests.cs

carlossanlop · 2020-07-21T17:57:42Z

.../UnitTests/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzerTests.cs

+        }
+
+        [Fact]
+        public async Task WrongPlatformStringsShouldHandledGracefully()


I'm trying to understand this test: Why do invalid strings still trigger the rule?

Good question, the purpose of this analyzer is not reporting the wrong string values, the purpose of this test is to check if the analyzer would crush or throw any unhandled exceptions with any different/wrong OS platform string values

...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.cs

buyaa-n · 2020-07-21T23:48:45Z

...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.cs

+            {
+                var typeName = WellKnownTypeNames.SystemRuntimeInteropServicesRuntimeInformation;
+
+                if (!context.Compilation.TryGetOrCreateTypeByMetadataName(typeName + "Helper", out var runtimeInformationType))


When we are able to consume the new APIs this mock class load will be removed

...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.cs

buyaa-n · 2020-07-21T23:58:01Z

...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.cs

+                        return;
+                    }
+
+                    if (guardMethods.IsEmpty || !(context.OperationBlocks.GetControlFlowGraph() is { } cfg))


guardMethods are new API so if it does not exist in the current compilation no need to do flow analysis, just report diagnostic for all methods in platformSpecificOperations dictionary

guardMethods are new API so if it does not exist in the current compilation

Right, but we could have bailed much earlier, no?

Don't we want to warn on builds not having those methods?

...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.cs

terrajobst · 2020-07-22T19:50:15Z

I've pulled down your PR and played with the analyzer. My feedback is below. I can file them as individual bugs as well.

Constructors aren't warned

Repro:

class Program
{
    static void Main(string[] args)
    {
        var x = new WindowsOnlyAPI();
        Console.WriteLine("Hello World!");
    }
}

[MinimumOSPlatform("win7.0")]
class WindowsOnlyAPI
{
}

Actual:

No diagnostic

Expected:

CA1416: 'WindowsOnlyAPI' requires win 7.0 version or later.

Analyzer should also handle IsOSPlatform

While IsOSPlatform doesn't include version information, it should use those checks to infer reachability for a platform in version 0.0.

Repro:

class Program
{
    static void Main(string[] args)
    {
        if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
            return;

        WindowsOnlyAPI.Test();
    }
}

[MinimumOSPlatform("win7.0")]
static class WindowsOnlyAPI
{
    public static void Test()
    {
    }
}

Actual:

CA1416: 'Test' requires win 7.0 version or later.

Expected:

No diagnostic

Incorrect version numbers are ignored

Repro:

class Program
{
    static void Main(string[] args)
    {
        if (!RuntimeInformation.IsOSPlatformOrLater("win7"))
            return;

        WindowsOnlyApi();
    }

    [MinimumOSPlatform("win7.0")]
    static void WindowsOnlyApi()
    {

    }
}

Actual:

CA1416: 'WindowsOnlyApi' requires win 7.0 version or later.

Expected:

Either infer that "win7" is "win7.0" or warn that "win7" isn't valid.

Logical or isn't handled properly

Repro:

class Program
{
    static void Main(string[] args)
    {
        if (RuntimeInformation.IsOSPlatformOrLater("win7.0") ||
            RuntimeInformation.IsOSPlatformOrLater("win7.1"))
        {
            WindowsOnlyApi();
        }
    }

    [MinimumOSPlatform("win7.0")]
    static void WindowsOnlyApi()
    {
    }
}

Actual:

CA1416: 'WindowsOnlyApi' requires win 7.0 version or later.

Expected:

No diagnostic

Feature: Handle OSVersion for Windows

We should consider special casing Windows (and maybe Unix) to also allow platform checks via the old Environment.OSVersion API.

Repro:

class Program
{
    static void Main(string[] args)
    {
        if (Environment.OSVersion.Platform == PlatformID.Win32NT &&
            Environment.OSVersion.Version >= new Version(7, 0))
        {
            WindowsOnlyApi();
        }
    }

    [MinimumOSPlatform("win7.0")]
    static void WindowsOnlyApi()
    {
    }
}

Actual:

CA1416: 'WindowsOnlyApi' requires win 7.0 version or later.

Expected:

No diagnostic

Local functions aren't processed

Repro:

class Program
{
    static void Main(string[] args)
    {
        void Test()
        {
            WindowsOnlyApi();
        }

        Test();
    }

    [MinimumOSPlatform("win7.0")]
    static void WindowsOnlyApi()
    {
    }
}

Actual:

No diagnostics

Expected:

CA1416: 'WindowsOnlyApi' requires win 7.0 version or later.

terrajobst

We should remove the TFM logic. The rest is just minor suggestions.

...yzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.Data.cs

terrajobst · 2020-07-22T20:21:20Z

...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.cs

+                        return;
+                    }
+
+                    if (guardMethods.IsEmpty || !(context.OperationBlocks.GetControlFlowGraph() is { } cfg))


guardMethods are new API so if it does not exist in the current compilation

Right, but we could have bailed much earlier, no?

...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.cs

buyaa-n · 2020-07-22T21:32:45Z

I've pulled down your PR and played with the analyzer. My feedback is below. I can file them as individual bugs as well.

Those are really interesting/good scenarios, thanks!

…m work

buyaa-n · 2020-07-28T00:07:50Z

...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.cs

+                                                                                      description: s_localizableObsoleteMessage,
+                                                                                      isPortedFxCopRule: false,
+                                                                                      isDataflowRule: false);
+        internal static DiagnosticDescriptor RemovedRule = DiagnosticDescriptorHelper.Create(RuleId,


@terrajobst @jeffhandley as i mentioned in the morning the Obsolete and Removed rules are work same as MinimumOS rule, i.e. if Obsolete attribute defined for an API then that API is restricted to only for that Platform. For now, i am gonna leave this implementation as is as we haven no any Obsolete/Removed attributes added into any API. The fix will come with the functionality update for the Unsupported attributes.

Thanks for the heads-up, @buyaa-n. For Preview 8, that sounds good to me. We'll then change the behavior in RC1 with the other changes.

Platform compatability analyzer draft

9e5e8f1

buyaa-n commented Jul 7, 2020

View reviewed changes

src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md Outdated Show resolved Hide resolved

buyaa-n added 6 commits July 8, 2020 19:00

Support more flow scenarios and add more tests

176adea

Enable value content analysis, add few more tests

9859abe

Merge conflict

0c84c22

Fix bug caused from merge

c7a4bc1

Merge branch 'master' of https://github.com/dotnet/roslyn-analyzers i…

8e84e4e

…nto plat_spec_an

Enum, property support. Use string comparison ever type, support string

42245ba

overload of guard method

buyaa-n force-pushed the plat_spec_an branch from 5d5f281 to 42245ba Compare July 16, 2020 18:18

buyaa-n added 2 commits July 17, 2020 09:58

Excclude conditional check, more refactoring

00ffd24

Add/fix tests

5a21e8e

jeffhandley mentioned this pull request Jul 18, 2020

[Meta] List of APIs considered for "Better Obsoletion" dotnet/runtime#33360

Closed

buyaa-n added 2 commits July 20, 2020 12:46

Add more tests

7e7e734

Osx equals mac and vise versa, add more tests and refactoring

e80c51a

carlossanlop reviewed Jul 21, 2020

View reviewed changes

.../UnitTests/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzerTests.cs Outdated Show resolved Hide resolved

carlossanlop reviewed Jul 21, 2020

View reviewed changes

buyaa-n added 3 commits July 21, 2020 15:00

Apply feedback, more refactoring

8c2f910

Merge conflict

00d17a2

Fix merge failures

4d3ee6e

buyaa-n commented Jul 21, 2020

View reviewed changes

...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.cs Outdated Show resolved Hide resolved

buyaa-n commented Jul 21, 2020

View reviewed changes

...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.cs Outdated Show resolved Hide resolved

buyaa-n commented Jul 21, 2020

View reviewed changes

buyaa-n commented Jul 22, 2020

View reviewed changes

...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.cs Show resolved Hide resolved

buyaa-n commented Jul 22, 2020

View reviewed changes

...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatabilityAnalyzer.cs Outdated Show resolved Hide resolved

terrajobst suggested changes Jul 22, 2020

View reviewed changes

buyaa-n added 2 commits July 23, 2020 14:05

Apply feedback

6d33a18

Add constructor support

08f98f1

buyaa-n added 2 commits July 24, 2020 10:03

Add local funtion, lambda expression tests

21e6e53

Add guarded local function, delegate, lambda, even tests and make the…

77c4acc

…m work

buyaa-n commented Jul 28, 2020

View reviewed changes

Add VB tests, some cleanup

32fa04c

buyaa-n closed this Aug 7, 2020

buyaa-n deleted the plat_spec_an branch August 7, 2020 21:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Platform compatibility analyzer draft #3

Platform compatibility analyzer draft #3

buyaa-n commented Jul 7, 2020 •

edited

Loading

carlossanlop Jul 21, 2020

buyaa-n Jul 21, 2020

buyaa-n Jul 21, 2020 •

edited

Loading

buyaa-n Jul 21, 2020

terrajobst Jul 22, 2020

buyaa-n Jul 22, 2020

terrajobst commented Jul 22, 2020 •

edited

Loading

terrajobst left a comment

terrajobst Jul 22, 2020

buyaa-n commented Jul 22, 2020

buyaa-n Jul 28, 2020

jeffhandley Jul 28, 2020

Platform compatibility analyzer draft #3

Platform compatibility analyzer draft #3

Conversation

buyaa-n commented Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buyaa-n Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terrajobst commented Jul 22, 2020 • edited Loading

Constructors aren't warned

Analyzer should also handle IsOSPlatform

Incorrect version numbers are ignored

Logical or isn't handled properly

Feature: Handle OSVersion for Windows

Local functions aren't processed

terrajobst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buyaa-n commented Jul 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buyaa-n commented Jul 7, 2020 •

edited

Loading

buyaa-n Jul 21, 2020 •

edited

Loading

terrajobst commented Jul 22, 2020 •

edited

Loading